[12.0][FIX] account_invoice_triple_discount: subtotal computation #876

SimoRubi · 2021-03-03T09:32:44Z

Steps (see also added test case)

In Sales > Configuration > Settings, enable 'discounts'
Create a sale order with one line having:
- price unit = 25
- quantity = 65
- discount = 50
- discount = 13

Before this PR
The line's subtotal is 707

After this PR
The line's subtotal is 706.88

This modification borrows the triple discount computation from https://github.com/OCA/sale-workflow/blob/361d4b039f281fe9293e4dcde30f5a8ce9c805bb/sale_triple_discount/models/sale_order_line.py and puts everything in abstract model, in order to be reused from sale order line (PR coming soon) and purchase order line.

todo

Default discount type from partner ([12.0][FIX] account_invoice_triple_discount: subtotal computation #876 (comment))
~~PR for purchase order~~ Does not make sense until the Odoo issue [ORM] Column dropped if moved to dependency's mixin odoo/odoo#67168 is solved (found during implementation of [12.0][REF] sale_triple_discount: subtotal computation sale-workflow#1495)
PR for sale order: [12.0][REF] sale_triple_discount: subtotal computation sale-workflow#1495

account_invoice_triple_discount/models/line_triple_discount.py

legalsylvain · 2021-03-03T10:05:51Z

account_invoice_triple_discount/models/line_triple_discount.py

+        'Discount 3 (%)',
+        digits=dp.get_precision('Discount'),
+    )
+    discounting_type = fields.Selection(


I don't understand where this field can be changed ? Should not be a setting on the related res.partner ? I mean some suppliers are using additive method and other multiplicative method.

Don't you think ?

This can be changed directly on the account invoice line's form, but it can be interesting to inherit that from the partner.
I'm currently investigating why Travis is still 🔴 (that's why this is still a Draft), after that I can try to add this new behavior.

While implementing this feature I noticed that it is not as trivial as it seems because default value for invoice line cannot rely on the line's fields (default methods are api.model), so I'd need to save it in the context etc..
I think that deserves its own PR, and here let's focus on the described issue, is that ok for you?

BTW Now that Travis is 🟢, I'll mark this PR as ready for reviews

well, I was more thinking on storing the discounting_type only on the res.partner or at least on the order/invoice method.
it will be a lighter implementation.
Do you think there are real cases when there are some line with additive method, and other with multiplicative ?

I mean, setting this value 50 times, if you have 50 lines will be a mess, in a UX point of view.

That is why I was talking about a default value for new lines: each of the 50 lines would have the default value inherited from the partner; and switching partner in the invoice would trigger the edit of all the invoice's lines.

I don't know about real cases of lines having different kinds of discounts but this is how sale orders + triple_discount currently work and it would be nice to have it in invoices too.

I added the logic explained above, can you update your review?

I don't know about real cases of lines having different kinds of discounts but this is how sale orders + triple_discount currently work and it would be nice to have it in invoices too.

Well if it's not a use case, it should be changed I think. Don't you ?

the problem with the current implementation is that it will create inconsistencies. I mean, it's possible to have "multiplicative" and invoice level and "additive" on line level.

you are adding new field on invoice / invoice.line. don't you think you should add a pre-migration script to populate correctly the database. I fear that an update of the current module on a production database will be slow. Don't you think ?

HaraldPanten · 2021-03-03T10:15:26Z

Might it be related to OCA/purchase-workflow#834 and my comment in #733 ?

SimoRubi · 2021-03-03T10:20:26Z

Might it be related to OCA/purchase-workflow#834 and my comment in #733 ?

It might be, I didn't know purchase's version of triple discount but looks like every module is re-implementing that logic in its own way.
This PR aims to centralize the logic in the new mixin class that will then be inherited from lines of account.invoice, sale.order and purchase.order.

HaraldPanten · 2021-03-03T10:42:30Z

Might it be related to OCA/purchase-workflow#834 and my comment in #733 ?

It might be, I didn't know purchase's version of triple discount but looks like every module is re-implementing that logic in its own way.
This PR aims to centralize the logic in the new mixin class that will then be inherited from lines of account.invoice, sale.order and purchase.order.

OK!

…t management

When `super` method is `api.one`, each line has to be processed separately in order to have cache consistency.

SimoRubi · 2021-06-16T13:32:57Z

@OCA/accounting-maintainers can this be merged?
@legalsylvain can you update your review?

Thanks!

SimoRubi · 2021-06-16T15:05:08Z

Last commit solves the same issue as OCA/sale-workflow#1618

… discount's precision

SimoRubi · 2021-08-31T15:57:51Z

@OCA/accounting-maintainers can this be merged or do I have to fix anything?
Thanks!

tafaRU · 2021-09-01T14:17:26Z

@SimoRubi could you please have a look at SimoRubi#1 ?

SimoRubi · 2021-11-29T10:08:09Z

@OCA/accounting-maintainers can this be merged or do I have to fix anything?
Thanks!

SimoRubi · 2021-11-29T10:09:25Z

@SimoRubi could you please have a look at SimoRubi#1 ?

I had a look and I think that cannot be merged as explained in SimoRubi#1 (review)

SimoRubi · 2022-03-08T13:09:35Z

@OCA/accounting-maintainers can this be merged or do I have to fix anything?
Thanks!

sergioeix · 2022-03-30T06:15:49Z

So this is happening on v13 #733 and i guess it happens on v14 too??

francesco-ooops · 2022-07-18T08:00:37Z

@legalsylvain could you review this one again? thanks

legalsylvain

Regarding Point 1, I think it can be changed later, but could you write a bullet in the ROADMAP.rst file to mention that topic ?
Also write a bullet to say that sale_triple_discount should be refactored to use new mixin introduced in that module.

https://github.com/OCA/sale-workflow/blob/14.0/sale_triple_discount/__manifest__.py

Regarding point 2 & 3, let me know your point of view.

francesco-ooops · 2022-07-26T08:09:13Z

@SirTakobi can you take a look at the recent review?

SirTakobi · 2022-07-29T14:02:35Z

@SirTakobi can you take a look at the recent review?

Thanks for letting me know of #876 (review).

I can't edit this PR because it is based on @SimoRubi's branch.
I'm currently not working on this topic so I won't be looking into this in the near future, if you want to make improvements or go on with the PR you can simply create another PR starting from https://github.com/SimoRubi/account-invoicing/tree/12.0-fix-account_invoice_triple_discount-subtotal_computation

github-actions · 2023-06-25T12:15:53Z

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

legalsylvain reviewed Mar 3, 2021

View reviewed changes

account_invoice_triple_discount/models/line_triple_discount.py Outdated Show resolved Hide resolved

legalsylvain reviewed Mar 3, 2021

View reviewed changes

SimoRubi marked this pull request as ready for review March 3, 2021 12:10

[FIX] account_invoice_triple_discount: Added mixin for triple discoun…

baa149f

…t management

SimoRubi force-pushed the 12.0-fix-account_invoice_triple_discount-subtotal_computation branch from daa1d21 to baa149f Compare March 3, 2021 12:13

This was referenced Mar 3, 2021

[12.0][REF] sale_triple_discount: subtotal computation OCA/sale-workflow#1495

Closed

[ORM] Column dropped if moved to dependency's mixin odoo/odoo#67168

Open

Fix cache management.

36f964c

When `super` method is `api.one`, each line has to be processed separately in order to have cache consistency.

SimoneVagile approved these changes Mar 9, 2021

View reviewed changes

SimoRubi added 2 commits March 23, 2021 11:25

Add default discount type for partner

d3a4e19

Add default discount type for invoice lines, from partner

03d42d4

SimoRubi requested a review from legalsylvain March 26, 2021 14:43

HaraldPanten mentioned this pull request May 25, 2021

[13.0][MIG] account_invoice_triple_discount #733

Closed

SimoRubi marked this pull request as draft June 16, 2021 14:13

SimoRubi marked this pull request as ready for review June 16, 2021 14:50

[FIX] account_invoice_triple_discount: Computed discount might exceed…

83c27b1

… discount's precision

SimoRubi force-pushed the 12.0-fix-account_invoice_triple_discount-subtotal_computation branch from a847827 to 83c27b1 Compare June 17, 2021 13:15

SimoneVagile approved these changes Jun 17, 2021

View reviewed changes

tafaRU mentioned this pull request Sep 1, 2021

[FIX] sale_triple_discount: restore original discount field's precision by using change_digit function SimoRubi/sale-workflow#1

Open

SirTakobi mentioned this pull request Jul 14, 2022

[14.0][MIG] l10n_it_fatturapa_out_triple_discount OCA/l10n-italy#2760

Closed

legalsylvain suggested changes Jul 18, 2022

View reviewed changes

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 25, 2023

github-actions bot closed this Jul 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[12.0][FIX] account_invoice_triple_discount: subtotal computation #876

[12.0][FIX] account_invoice_triple_discount: subtotal computation #876

SimoRubi commented Mar 3, 2021 •

edited

Loading

legalsylvain Mar 3, 2021

SimoRubi Mar 3, 2021

SimoRubi Mar 3, 2021

legalsylvain Mar 3, 2021

SimoRubi Mar 3, 2021

SimoRubi Mar 26, 2021

legalsylvain Jul 18, 2022 •

edited

Loading

HaraldPanten commented Mar 3, 2021

SimoRubi commented Mar 3, 2021

HaraldPanten commented Mar 3, 2021

SimoRubi commented Jun 16, 2021

SimoRubi commented Jun 16, 2021

SimoRubi commented Aug 31, 2021

tafaRU commented Sep 1, 2021

SimoRubi commented Nov 29, 2021

SimoRubi commented Nov 29, 2021

SimoRubi commented Mar 8, 2022

sergioeix commented Mar 30, 2022

francesco-ooops commented Jul 18, 2022

legalsylvain left a comment

francesco-ooops commented Jul 26, 2022

SirTakobi commented Jul 29, 2022

github-actions bot commented Jun 25, 2023

[12.0][FIX] account_invoice_triple_discount: subtotal computation #876

[12.0][FIX] account_invoice_triple_discount: subtotal computation #876

Conversation

SimoRubi commented Mar 3, 2021 • edited Loading

legalsylvain Mar 3, 2021

Choose a reason for hiding this comment

SimoRubi Mar 3, 2021

Choose a reason for hiding this comment

SimoRubi Mar 3, 2021

Choose a reason for hiding this comment

legalsylvain Mar 3, 2021

Choose a reason for hiding this comment

SimoRubi Mar 3, 2021

Choose a reason for hiding this comment

SimoRubi Mar 26, 2021

Choose a reason for hiding this comment

legalsylvain Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

HaraldPanten commented Mar 3, 2021

SimoRubi commented Mar 3, 2021

HaraldPanten commented Mar 3, 2021

SimoRubi commented Jun 16, 2021

SimoRubi commented Jun 16, 2021

SimoRubi commented Aug 31, 2021

tafaRU commented Sep 1, 2021

SimoRubi commented Nov 29, 2021

SimoRubi commented Nov 29, 2021

SimoRubi commented Mar 8, 2022

sergioeix commented Mar 30, 2022

francesco-ooops commented Jul 18, 2022

legalsylvain left a comment

Choose a reason for hiding this comment

francesco-ooops commented Jul 26, 2022

SirTakobi commented Jul 29, 2022

github-actions bot commented Jun 25, 2023

SimoRubi commented Mar 3, 2021 •

edited

Loading

legalsylvain Jul 18, 2022 •

edited

Loading